Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CATALOG-2408 - Allow template property for optionChange #79

Merged
merged 1 commit into from
Feb 21, 2018

Conversation

bookernath
Copy link
Contributor

I have a use case where I need to return part of the product options payload as rendered HTML. Therefore, I need to be able to send a template property.

@bigbot
Copy link

bigbot commented Feb 20, 2018

Autotagging @bigcommerce/storefront-team

@@ -21,9 +21,9 @@ export default class extends Base
* @param {Object} params
* @param callback
*/
optionChange(productId, params, callback) {
optionChange(productId, params, template=null, callback) {
Copy link
Contributor

@lord2800 lord2800 Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do something like

if (typeof template === 'function') {
    let temp = callback;
    callback = template;
    template = temp;
}

immediately at the top of the function. That gives you a modicum of backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Gonna test it now...

@bookernath bookernath force-pushed the optionChange_template branch 3 times, most recently from 4ed83ba to 573aa1f Compare February 21, 2018 03:03
@bookernath
Copy link
Contributor Author

bigcommerce/cornerstone#1167 related cornerstone PR

@bookernath bookernath force-pushed the optionChange_template branch 2 times, most recently from 25396da to 7693d8f Compare February 21, 2018 05:17
@bookernath
Copy link
Contributor Author

@lord2800 linter is happy, re-review?

@bookernath
Copy link
Contributor Author

I confirmed this doesn't break existing Cornerstone if I use it without any code changes in the theme, so I believe it to be backwards compatible.

@bookernath bookernath force-pushed the optionChange_template branch from 7693d8f to d30177d Compare February 21, 2018 17:43
@bookernath bookernath force-pushed the optionChange_template branch from d30177d to 584d061 Compare February 21, 2018 17:50
@lord2800 lord2800 merged commit 1348d07 into bigcommerce:master Feb 21, 2018
@bookernath bookernath deleted the optionChange_template branch November 7, 2018 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants